Skip to content

Fix the three extraction regressions: false-green whole-repo check, review workspace root, displayed defaults - #6

Closed
schickling-assistant wants to merge 3 commits into
mainfrom
schickling-assistant/2026-08-08-fix-extraction-regressions
Closed

Fix the three extraction regressions: false-green whole-repo check, review workspace root, displayed defaults#6
schickling-assistant wants to merge 3 commits into
mainfrom
schickling-assistant/2026-08-08-fix-extraction-regressions

Conversation

@schickling-assistant

@schickling-assistant schickling-assistant commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Scope of the consuming repository's exposure — measured, after an earlier overstatement.
An earlier revision of this body claimed a green check on the consuming repository was
non-evidence for decision-shape enforcement until this PR and its repin landed. That was
measured and it does not hold.
The host CLI passes the corpus root explicitly, so
root_or_default never reaches the fallback this PR restores, and an A/B on identical host code
across both pins catches a planted malformed decision either way — 4 diagnostics, exit 1, on
both. No enforcement path in the consuming repository aims a check at a repository root; all of
them name the corpus. A green check there is evidence.

What survives: aim the standalone binary at a repository root and the shape pass really is
absent — 0 diagnostics of that rule pre-fix against 4 post-fix. Even then it is not a false
green
: that run exits 1 on ~955 other diagnostics. This PR fixes a real defect in the
standalone tool; it is not closing a live exposure downstream.

Fixes the three behaviours reported in #4. One of them was a false green; the other two
are reported here with what actually reproduced and what did not.

1. Whole-repository check returned a false green (high)

meta_vrs_decision_dir resolved only <root>/.decisions, so check <repo-root> found no
decision directory, skipped the shape pass entirely and reported a clean tree. Restored the
fallback to the conventional corpus position.

The fallback is strictly additive: it is consulted only when <root>/.decisions is
absent, and it names a path a differently laid out repository does not have. It can only
ever add enforcement, never remove or redirect it — so the "silently wrong for another
layout" objection in the removed comment does not hold: such a repository has no
context/vrs/.decisions, and nothing changes for it.

Scope question, flagged rather than decided. #4 frames this as the contract documenting
whole-repository scope while the code implements corpus scope. I could not find that text.
Searching this repository for whole-repository/repo-wide/repository-root phrasings turns up
nothing that says check at a repository root enforces decision shape, and the surrounding
material leans corpus-scoped: context/cli/spec.md:63 documents the default path as
context/vrs, and the argument's own help has always read "VRS root to check". The one
repository-relative statement is context/cli/spec.md:174, which names the rule's subject as
context/vrs/.decisions/.

So the contract is not in conflict here, and this is a documentation clarification rather than a
choice: the restored fallback consults both paths, so the whole-repository reading and the
narrower corpus-scoped line in context/cli/spec.md are each satisfied. Nothing needs to be
narrowed. I did not touch context/cli/*.md.

Correction — "removes a false green either way" is too strong, and independent verification
falsified it.
It restores the missing enforcement for the context/vrs layout — genuinely a clean
pass on a bad tree in the harness above, though on a real populated tree the run fails on other
rules and the loss shows up as one silently absent rule rather than a green. It does not restore
it for this repository, because
CORPUS_SENTINEL is hardcoded to context/vrs while this repo's corpus is intent/. Aimed at
this repo's root, the decision-shape pass is still silently absent. That residual gap predates
this PR (the hardcode is already at da5ac9f1b), is currently latent — all 36 records scan clean
and CI aims at the corpus — and is not a clean-bill-of-health green here, since the repo-root
run exits 1 on 8 other diagnostics. It is tracked separately as #8 and is deliberately out of
scope for this change.

The test is the point

decision_shape_is_enforced_when_check_is_aimed_at_the_repository_root invokes check at the
repository root with a malformed decision at context/vrs/.decisions/. On unfixed code it
fails with an empty diagnostics array — a clean pass on a tree containing a malformed
decision, which is exactly the reported failure.

Every pre-existing helper hands check the context/vrs path itself, which resolves the
decision directory however the tool locates it. That is why the regression was invisible, and
why a test on the corpus path would reproduce the blind spot instead of catching it. The new
check_at helper exists so the aimed-at path is a parameter rather than baked in.

2. review_workspace lost its context/vrs sentinel (medium) — reproduced, partly

The mechanism reproduces. With only the .git probe, a corpus in a tree without .git
resolves to the corpus root rather than the repository root
(review_workspace_finds_the_repository_by_corpus_layout_when_there_is_no_git fails on
unfixed code, returning the corpus). That root becomes the review backend's --cwd, so the
reviewer is handed the corpus alone and loses the repository the corpus describes. Restored
the sentinel as a second probe.

The escape-boundary consequence does not reproduce, and I would rather correct that here
than repeat it. #4 says target artifacts elsewhere in the repository would be rejected. They
would not: target files are collected by markdown_files(invocation.root)
(crates/intent/src/lib.rs:432), which descends from the corpus root only, and the workspace
is always that same root or an ancestor of it. Every target file therefore starts with the
workspace under either resolution, so the check at crates/intent/src/lib.rs:449 cannot fire
for review. The reported harm is real; that particular mechanism for it is not.

Neither probe is a guess that can mislead — both name a directory that is present or is not.
The last-resort corpus fallback keeps its existing test, unmodified; only its now-stale
comment changed.

3. Displayed defaults disappeared from --help and completions (medium)

The defaults moved out of the command tree so the standalone tool and its embedding host could
carry different ones. That constraint is real, but it does not require giving up the displayed
metadata: Defaults::command injects the caller's defaults onto the arguments themselves, and
Defaults::parse parses through that same tree. check --help now reads
[ROOT] VRS root to check [default: .] again, and completions carry it.

run_with still resolves an absent argument, so callers constructing the types directly are
unaffected. Setting default_value means args.root now arrives as Some on the binary
path rather than None, which changes nothing observable: it is read only through
root_or_default/fixtures_or_default, and both resolve it to the same value either way.

Half of this lands here, and the other half cannot. --help is rendered from whichever
command tree the caller builds. The standalone binary now builds its through
Defaults::command, so its help is fixed and tested. An embedding host parses VrsCli
through its own command tree and then calls run_with, so its help gains the defaults
only once it adopts Defaults::command/Defaults::parse too — a one-line change, but one
that belongs in the host's repository, not this one. #4 reports the symptom across
check|graph|review|review-fixtures --help; this PR supplies the mechanism and fixes the
standalone surface, and the host side is a separate follow-up rather than something silently
covered here.

This needs clap's string feature: the defaults are runtime values, and without it
default_value accepts only &'static ones — which is the constraint that pushed them out of
the command tree originally. No new dependency, and Cargo.lock is unchanged.

Two tests, both failing without the change:

  • help_advertises_the_default_root_the_run_will_use drives the shipped binary. This
    matters: the defect is a default that exists after parsing but never reaches the command
    tree --help renders, so a test building its own Command would pass while the real
    --help stayed empty. Verified by reverting main.rs alone — the test fails.
  • a_callers_default_reaches_the_help_it_renders covers an embedding caller's own
    context/vrs default, which is the case the constraint was about.

Noted, not fixed

  • crates/intent/src/lib.rs emits schema_version: "axe.vrs.check.v1", but
    context/cli/spec.md does not mention axe.vrs.check.v1 anywhere — its envelope
    enumeration is incomplete. Left alone deliberately: a rename is in flight across those
    strings and would conflict.
  • Follow-up: check_root skips the decision-shape pass silently whenever no decision
    directory is found (crates/intent/src/lib.rs:1085). Restoring the fallback fixes the
    reported case, but "found no decisions anywhere" still exits 0 and is indistinguishable from
    a clean tree. Making that observable needs a new diagnostic class, which is more than this
    change should carry.
  • Follow-up: the reusable engine is still coupled to the clap request types. Decoupling it
    is not a contained change and is not attempted here.
  • CORPUS_SENTINEL hardcodes context/vrs at two sites — decision-dir skip and repo-root discovery #8 — this repository's own layout is still not covered, and this PR does not cover it.
    CORPUS_SENTINEL is hardcoded to context/vrs; this repo's corpus is intent/. Aimed at the
    repository root, decision-shape enforcement is silently absent — proven by execution at this head
    with a planted malformed decision: intent check . reports 0 VRS.ENF.meta-decision-shape
    diagnostics where intent check intent reports 4, same binary, same file. It is not a
    whole-run false green here: the repo-root run exits 1 on 8 unrelated diagnostics. The hardcode
    predates this PR, it is latent today (all 36 records scan clean and CI aims at the corpus), and
    the fix belongs with a fixture on this repo's shape rather than a generic corpus probe — decision
    0022 deliberately scopes strict enforcement to one designated corpus, and a "find whatever holds
    VRS artifacts" probe is the broadening it rejected. Tracked in CORPUS_SENTINEL hardcodes context/vrs at two sites — decision-dir skip and repo-root discovery #8.

Verification

nix flake check passes locally — fmt, clippy, intent, help, and reads-the-corpus.
22 tests green (10 unit, 12 integration).

Correction — "each new test was confirmed to fail on unfixed code" is not accurate for all of
them, and independent verification established which.
Three genuinely fail against da5ac9f1b:
decision_shape_is_enforced_when_check_is_aimed_at_the_repository_root (empty diagnostics array),
help_advertises_the_default_root_the_run_will_use, and
review_workspace_finds_the_repository_by_corpus_layout_when_there_is_no_git (returns the corpus
rather than the repository root). The fourth,
a_callers_default_reaches_the_help_it_renders, does not compile against unfixed code — there
is no command method to call — so it cannot fail as a test. It is a real regression test going
forward; it is not evidence that the behaviour was broken before, and the original wording claimed
it was.

Posted on behalf of @schickling
field value
agent_name unknown
agent_tool Claude Code
agent_tool_version 2.1.220
agent_runtime Claude Code 2.1.220
agent_session_lookup unavailable
tooling_profile dotfiles@unknown-dirty

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ee2cc2afe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/intent/src/lib.rs
Comment thread crates/intent/src/lib.rs
@schickling-assistant
schickling-assistant marked this pull request as draft August 9, 2026 09:49
…oots

`meta_vrs_decision_dir` resolved only `<root>/.decisions`, so `check <repo-root>`
found no decision directory, skipped the shape pass and reported a clean tree —
a malformed decision under `context/vrs/.decisions/` passed. A false green is the
worst failure mode an enforcement tool has: it is indistinguishable from a run
that found nothing to report.

The fallback is strictly additive. It is consulted only when `<root>/.decisions`
is absent, and it names a path a differently laid out repository does not have, so
it can only add enforcement, never remove or redirect it.

No test could have caught this: every helper on both sides passes `context/vrs`
directly, which resolves the decision directory however the tool locates it. The
new test aims `check` at the repository root instead, and fails on unfixed code
with an empty diagnostics array.

agent-tool: Claude Code
agent-tool-version: 2.1.220
agent-runtime: Claude Code 2.1.220
agent-session-lookup: unavailable
tooling-profile: dotfiles@unknown-dirty
…kspace

`review_workspace` probed only for `.git`, so a tree without one resolved to the
corpus root. That root becomes the review backend's `--cwd`, so the reviewer was
handed the corpus alone and lost the repository the corpus describes. Ordinary
worktrees have `.git` and mask this entirely, which is why no interactive run
reaches it — only a vendored or exported tree, or a build sandbox, does.

Neither probe is a guess that can mislead: both name a directory that is present
or is not, and a repository laid out differently falls through to the corpus as
before. That last-resort branch keeps its test; only its stale comment moved.

The reported escape-boundary consequence does NOT reproduce, and the report is
corrected here rather than repeated: target artifacts are collected from the
corpus root, and the workspace is always that root or an ancestor of it, so the
boundary check cannot reject them under either resolution.

The corpus position is now one named constant shared with the decision-directory
lookup, so the two cannot drift apart.

agent-tool: Claude Code
agent-tool-version: 2.1.220
agent-runtime: Claude Code 2.1.220
agent-session-lookup: unavailable
tooling-profile: dotfiles@unknown-dirty
The corpus defaults moved out of the command tree so the standalone tool and its
embedding host could carry different ones, and were applied after parsing instead.
Bare invocation still resolved correctly, but `check|graph|review|review-fixtures
--help` stopped advertising them and generated completion metadata lost them — so
the documented default and the displayed one could disagree with nothing failing.

Caller-supplied defaults do not actually require giving up the displayed metadata:
`Defaults::command` injects them onto the arguments themselves, and
`Defaults::parse` parses through that same tree. The binary now parses that way, so
what `--help` advertises is what the run will use. `run_with` still resolves an
absent argument, so callers that build the types directly are unaffected.

This needs clap's `string` feature: the defaults are runtime values, and without it
`default_value` takes only `&'static` ones — which is the constraint that pushed
them out of the command tree to begin with.

Both tests fail without the change. The integration test drives the SHIPPED binary,
because a test that built its own command tree would reproduce the blind spot
rather than catch it; the unit test covers an embedding caller's own default.

agent-tool: Claude Code
agent-tool-version: 2.1.220
agent-runtime: Claude Code 2.1.220
agent-session-lookup: unavailable
tooling-profile: dotfiles@unknown-dirty
@schickling-assistant

Copy link
Copy Markdown
Contributor Author

Superseded by #11, which carries these fixes forward semantically on current main while converging the corpus layout required by #8: #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant